feat: add debounce and throttle utilities - #6
Conversation
There was a problem hiding this comment.
The PR adds two simple but functional debounce and throttle utilities. The logic is correct for the happy path, but the implementation uses any types where the project convention (see src/utils/package-root.ts) avoids them entirely, and the new file has zero test coverage in a project that maintains 22 test files. Risk level: low-medium — the functions are small and isolated, but untyped internals and missing tests leave edge cases (negative/NaN delays, cross-platform timer types) unprotected.
3 finding(s): 0 error, 2 warning, 1 info
[WARNING] style
File: src/utils/debounce.ts:2
let timer: any uses a bare any type. The adjacent utility package-root.ts avoids any entirely, so this departs from project convention. It also masks the actual type returned by setTimeout, which differs between Node (NodeJS.Timeout) and browser (number) runtimes.
Suggestion: Replace let timer: any with let timer: ReturnType<typeof setTimeout> | undefined; and initialize it as undefined so clearTimeout on first call is explicitly safe.
[WARNING] testing
File: src/utils/debounce.ts
No tests exist for debounce or throttle. The project has 22 test files covering other modules. These utilities have non-trivial edge cases: timer cleanup, rapid successive calls, and boundary timing.
Suggestion: Add a test file (e.g. test/utils/debounce.test.ts) covering: (1) debounce delays execution and only fires the last call, (2) throttle limits calls within the interval, (3) debounce resets the timer on re-entry, (4) behavior with zero or negative delay values.
[INFO] error-handling
File: src/utils/debounce.ts:1
delayMs and intervalMs are not validated. Passing NaN or a negative number causes setTimeout to fire immediately and throttle to execute on every call, which may surprise callers.
Suggestion: Consider guarding with if (!Number.isFinite(delayMs) || delayMs < 0) throw new RangeError('delayMs must be a non-negative finite number') (similarly for intervalMs), or document the behavior in JSDoc.
Reviewed by pi-relay · View full run
| @@ -0,0 +1,18 @@ | |||
| export function debounce<T extends (...args: any[]) => void>(fn: T, delayMs: number): T { | |||
| let timer: any; | |||
There was a problem hiding this comment.
[WARNING] style
let timer: any uses a bare any type. The adjacent utility package-root.ts avoids any entirely, so this departs from project convention. It also masks the actual type returned by setTimeout, which differs between Node (NodeJS.Timeout) and browser (number) runtimes.
Suggestion: Replace let timer: any with let timer: ReturnType<typeof setTimeout> | undefined; and initialize it as undefined so clearTimeout on first call is explicitly safe.
| @@ -0,0 +1,18 @@ | |||
| export function debounce<T extends (...args: any[]) => void>(fn: T, delayMs: number): T { | |||
There was a problem hiding this comment.
[INFO] error-handling
delayMs and intervalMs are not validated. Passing NaN or a negative number causes setTimeout to fire immediately and throttle to execute on every call, which may surprise callers.
Suggestion: Consider guarding with if (!Number.isFinite(delayMs) || delayMs < 0) throw new RangeError('delayMs must be a non-negative finite number') (similarly for intervalMs), or document the behavior in JSDoc.
AI Review: Fixes AppliedReviewed and found 3 issue(s). All addressed. Changes MadeChanges made:
Skipped:
Verification passed. |
|
Test PR — verified npm link workflow. Closing. |
Testing npm link workflow.